Repository navigation
feat(tron): return live data from listAccountAssets and getAccountBalances - #388
Conversation
3670b04 to
baff8e5
Compare
listAccountAssets and getAccountBalances
listAccountAssets and getAccountBalanceslistAccountAssets and getAccountBalances
ebc77b5 to
8133f76
Compare
822392c to
06f231f
Compare
| async fetchAccountAssets(account: KeyringAccount): Promise<AssetEntity[]> { | ||
| const results = await Promise.all( | ||
| account.scopes.map((scope) => | ||
| this.fetchAccountAssetsByScope(account, scope as Network), |
There was a problem hiding this comment.
Should we relax fetchAccountAssetsByScope's scope argument type to accept ${string}:${string}, or add a type guard here so we don't need this cast?
There was a problem hiding this comment.
Hmm very good idea, as is definitely to be avoided. Let me give it a try.
There was a problem hiding this comment.
The problem is we are using Network everywhere as the required type. So basically, somehow, we need to convert the input on handlers and then have Network internally which is more specific
There was a problem hiding this comment.
if we want to keep using Network we should add a type guard to narrow down the type from ${string}:${string} so that we throw on unsupported scopes while also making typescript happy - this function seems to be a good place to do it since we take the scope value directly from the KeyringAccount
There was a problem hiding this comment.
100%. I am going to look around the code and see if it makes sense to add some more of that here or open a PR right next to it
There was a problem hiding this comment.
It should be no more than three lines of code before this Promise.all call, so IMO we can do it in this PR, but your call
There was a problem hiding this comment.
Due to the downstream effects of the type change I am going to address in a follow up PR with more type hardening on all things related to Network, scopes, etc.
06f231f to
5612e01
Compare
5612e01 to
fe369ca
Compare
fe369ca to
67d43ca
Compare
67d43ca to
bdbe277
Compare
There was a problem hiding this comment.
i have a question
when we use listAccountAssets and getAccountBalances from controller
means there will be a case that they dont wanna to use accounts API (becoz accounts API failed)
if im not miss taken, the live data from those 2 methods are returning the result from accounts API
wdyt?
| account: KeyringAccount, | ||
| scope: Network, | ||
| ): Promise<AssetEntity[]> { | ||
| if (await this.#shouldReturnAssetsFromCore()) { |
There was a problem hiding this comment.
Not sure if i understand correct
shouldReturnAssetsFromCore is reading FF and fetchAccountAssetsByScope is reading data base on this condtion
but there are 2 fallbacks
1 is fallback by FF
2 is fallback becoz accounts API fail and then fallback to snap to get balance, the FF untouch
There was a problem hiding this comment.
Good catch — this is exactly the coupling we wanted to avoid. Fixed in 1a040f3: fetchAccountAssets (the keyring path) now always hits TronGrid through the Snap adapter regardless of flag state, so the response shape is stable. The flag-gated fan-out stays only in fetchAccountAssetsByScope, which is now exclusive to the asset synchronization flow in AccountsService.
b0c96b3 to
12924fb
Compare
|
|
||
| - Show "Estimated changes are not available" instead of "No estimated changes" when the transaction scan returns an error result, such as for a malformed transaction ([#396](https://github.com/MetaMask/internal-snaps/pull/396)) | ||
| - Report the MetaMask origin as lowercase `metamask` instead of `MetaMask` for MetaMask-initiated operations, so the origin matches the value used by the other non-EVM snaps and granted to the keyring methods, and so transaction scan requests are attributed to `https://metamask.io`. The confirmation UI keeps displaying `MetaMask`. ([#392](https://github.com/MetaMask/internal-snaps/pull/392)) | ||
| - Show "Estimated changes are not available" instead of "No estimated changes" when the transaction scan returns an error result, such as for a malformed transaction ([#396](https://github.com/MetaMask/internal-snaps/pull/396)) |
There was a problem hiding this comment.
main has this line duplicated
There was a problem hiding this comment.
Match the handler names properly
|
Ready for another look @mikesposito @stanleyyconsensys |
stanleyyconsensys
left a comment
There was a problem hiding this comment.
just a nit comment for improvement
|
|
||
| const assetsList = await this.#assetsService.getAccountAssets(accountId); | ||
| const assetsList = | ||
| await this.#assetsService.fetchAccountAssetsFromTrongrid(account); |
There was a problem hiding this comment.
nit:
shall we consider adding this.#assetsService.fetchAccountAssetsFromTrongrid with InMemoryCache
as Caller do listAccountAsset first then getAccountBalances
So those SNAP api method use the same tron grid API, we can have a bit optizme to reduce the network call?
e.g
- listAccountAsset: always return live data + save into in memory cache with expire
- getAccountBalances: read cache first then read live data if cache not exist
There was a problem hiding this comment.
I would start without it. Just to make sure everything works on main. And then optimize it
…etAccountBalances
…ia TronGrid The keyring methods (listAccountAssets, getAccountBalances) fetched live data through a feature-flag-gated path: when the assets migration flag was active the fetch routed through the CoreAssetsAdapter, which only returns snap-owned assets, changing the response shape based on flag state. fetchAccountAssets now always hits TronGrid through the Snap adapter regardless of the migration stage; the migration-aware fan-out stays in AssetsService.fetchAccountAssetsByScope for the asset synchronization flow.
The CoreAssetsAdapter fetched live assets by calling TronGrid directly (account info, account resources, staking rewards) and rebuilding the special-asset extraction stack locally, duplicating the fetch pipeline the AssetsController already owns. fetchAccountAssets in the CoreAssetsAdapter now goes through the AssetsController's getAssets action with forceUpdate and bypassServerCache, so neither client nor server caches are used and the special assets flow is owned by the controller pipeline. The Snap adapter keeps the direct TronGrid fetch for the non-migration path, and fetchAccountAssetsFromTrongrid stays as the flag-independent fallback. The asset synchronization flow fetches once per account instead of once per account-and-scope combination.
…ame validation structs
12924fb to
a1db237
Compare
|



Explanation
listAccountAssetsandgetAccountBalancesin the Tron Snap previously returned whatever was persisted in the Snap's state, so the data was only as fresh as the last cronjob indexation. This is needed so the assets controller can use the Snap as the data source for reconcile when the asset migration feature flag switches back.Changes:
getAccountAssets(listAccountAssets) andgetAccountBalancesin the keyring now fetch live assets and balances from the chain (viafetchAccountAssetsFromTrongrid) instead of reading persisted state. Fetch failures propagate to the caller.getAccountBalancesonly fetches live data for the scopes the requested assets belong to, avoiding unnecessary chain calls.AssetsService.fetchAccountAssets, a migration-aware live fetch: when the migration is active it goes through the AssetsController fetch pipeline (getAssetswithforceUpdateandbypassServerCache, so the Core messenger now includesAssetsControllerGetAssetsAction), otherwise it hits TronGrid directly through the Snap adapter.AssetsService.fetchAccountAssetsFromTrongrid, a fallback that always fetches live data from TronGrid regardless of the migration feature flag.CoreAssetsAdapterno longer queries TronGrid directly; its live fetch goes through the AssetsController pipeline. The TronGrid extraction logic (staking, bandwidth, energy, etc.) now only lives in the Snap adapter.AccountsService.synchronizeAssetsnow fetches assets per account across all the account's scopes (viafetchAccountAssets) instead of per account ×activeNetworksscope combination.Behavior by assets migration feature flag
The flag is
SNAPS_ASSETS_MIGRATION_FLAG_KEYS.tron(any migration stage other thanOffcounts as ON), checked inAssetsService.#shouldReturnAssetsFromCore().Keyring.getAccountAssetsKeyring.getAccountBalancesAssetsService.getAccountAssetsAssetsService.getAccountAssetsByIDsAssetsService.getAccountAssetByIDAssetsService.fetchAccountAssetsgetAssetswithforceUpdate+bypassServerCache)AssetsService.fetchAccountAssetsFromTrongridAssetsService.saveManyNote: when the flag is ON, the keyring methods fetch live data via TronGrid while the controller path fetches via the AssetsController pipeline — two different live-data sources, and the keyring one may include assets the controller would not return (ownership filtering).
References
Ticket: WPN-2217
Checklist